Very interesting, I didn't expect to be able to do this! Is this a solution to #1285?

Looking at the issue, I think this could be a solution for it!

Hmm interesting.

This wouldn't be a satisfactory solution to all of #1285, because this appears to fire up a whole XML parser for each one of these widgets, which is going to be expensive. That's acceptable in this context, where there's only one of these at a time (and it isn't a very common part of the UI anyway). It wouldn't be OK for something that could appear in, say, message content, where you might scroll past a lot of them.

I've written up a small TextWithLink widget which does the thing we need here more directly. Pushing that to this branch as a pair of additional commits on top:
c33ee61 text: Introduce TextWithLink widget
93e8919 (squash) Use our TextWithLink instead of package:styled_text

I think for the other cases we need for #1285, I'll be happier with further developing that widget than with what package:styled_text is doing. In particular the thing that I feel least confident about in what's missing from TextWithLink is this:

/// TODO(#1285): Integrate this with l10n so that the markup can be parsed
///   from the constant translated string, with placeholders for any variables,
///   rather than the string that results from interpolating variables.
///   That way it'll be fine to interpolate variables with arbitrary text.

but package:styled_text doesn't handle that either.

An intermediate commit has:

  @override void onPressed() {
    // TODO: open read receipts sheet
  }

So it shows the button but the button doesn't do anything. That's a buggy state, so let's avoid it.

That commit is quite small, so we can just squash it into the later commit that gives this button something to do.

This name is very general. It's hard for me to tell what sort of situations this is meant for, or what it does.

If I want plain text, that's what Text does, right? It's not clear how the bottom sheet interacts with that.

What about the new name BottomSheetInfoText?

That does suggest more information. Can you explain a bit more in the widget's doc?

Updated its doc.

This chain of ?: would be best as a switch (status). That way it's clear we handle all the possible values.

Does this Center end up doing anything in the three cases other than CircularProgressIndicator? I suspect it doesn't.

It also centers the plain text in the sheet, especially vertically.🙂

In the current revision, I believe this Center only has an effect in the FetchStatus.loading state, and otherwise it does nothing, which is confusing. (BottomSheetEmptyContentPlaceholder takes care of positioning the "no-read-receipts" message in the error and success cases.)

I think it would be reasonable to give that new widget the job of showing a loading indicator as appropriate, and then this Center can go away. Could give it a loading param, requiring either that or message but not both`, and have it show the loading spinner in the place where message would go; something like:

--- lib/widgets/action_sheet.dart
+++ lib/widgets/action_sheet.dart
@@ -182,7 +182,11 @@ class BottomSheetHeader extends StatelessWidget {
   }
 }
 
-/// A "no content here" message for when a bottom sheet has no content to show.
+/// A placeholder for when a bottom sheet has no content to show.
+///
+/// Pass [message] for a "no-content-here" message,
+/// or pass true for [loading] if the content hasn't finished loading yet,
+/// but don't pass both.
 ///
 /// Show this below a [BottomSheetHeader] if present.
 ///
@@ -192,26 +196,35 @@ class BottomSheetHeader extends StatelessWidget {
 // TODO(design) we don't yet have a design for this;
 //   it was ad-hoc and modeled on [PageBodyEmptyContentPlaceholder].
 class BottomSheetEmptyContentPlaceholder extends StatelessWidget {
-  const BottomSheetEmptyContentPlaceholder({super.key, required this.message});
+  const BottomSheetEmptyContentPlaceholder({
+    super.key,
+    this.message,
+    this.loading = false,
+  }) : assert(message == null || !loading);
 
-  final String message;
+  final String? message;
+  final bool loading;
 
   @override
   Widget build(BuildContext context) {
     final designVariables = DesignVariables.of(context);
 
-    return Padding(
-      padding: EdgeInsets.fromLTRB(24, 48, 24, 16),
-      child: Align(
-        alignment: Alignment.topCenter,
-        child: Text(
+    final child = loading
+      ? CircularProgressIndicator()
+      : Text(
           textAlign: TextAlign.center,
           style: TextStyle(
             color: designVariables.labelSearchPrompt,
             fontSize: 17,
             height: 23 / 17,
           ).merge(weightVariableTextStyle(context, wght: 500)),
-          message)));
+          message!);
+
+    return Padding(
+      padding: EdgeInsets.fromLTRB(24, 48, 24, 16),
+      child: Align(
+        alignment: Alignment.topCenter,
+        child: child));
   }
 }

nit: widget parameters should always be named (with very rare exceptions like Text)

The child widget here calls this argument pageContext, but the context we're passing looks like just its own parent's context. If this context works, then I expect the child's own context would work equally well.

(This is also an example of why it's good for widget parameters to be named — if this said pageContext: context, then the contrast would jump out more.)

This should go in lib/api/ :slightly_smiling_face:

Hmm interesting.

This wouldn't be a satisfactory solution to all of #1285, because this appears to fire up a whole XML parser for each one of these widgets, which is going to be expensive. That's acceptable in this context, where there's only one of these at a time (and it isn't a very common part of the UI anyway). It wouldn't be OK for something that could appear in, say, message content, where you might scroll past a lot of them.

icons: Add check_check icon, from Web Figma file

I started a discussion on this; I don't think there's a change to make right now, but please link to it in the commit message for context: #mobile-design > read-receipts icon in #F1706 @ 💬

action_sheet: Rename BottomSheetHeaderPlainText to BottomSheetInfoText

This commit decreases the number of widgets that

• declare the job of following one specific source in the Figma,
• do that job correctly, and
• have an implementation that's easy to compare to that source. 🙂

Let's make a new widget for the new not-header thing, and keep this widget for a header. This would be an example of the principle "prefer duplication over the wrong abstraction", I think 🙂, which Greg taught me about a while ago: https://chat.zulip.org/#narrow/sender/2187-Greg-Price/search/wrong.20abstraction

aside

I could more easily imagine BottomSheetHeaderPlainText growing a "title/body" variant, if that's helpful for following this part of the Figma—

; it would still have the "header" abstraction, which seems right. In fact, that might be helpful—the Figma's 18px horizontal padding kind of looks like a mistake to me; the plain-text version has 16px. A shared widget would be a natural place for a code comment that mentions both kinds of headers. We'd want to make sure the widget still has a clear, coherent interface, with a link to the Figma source for the new behavior.

I could more easily imagine BottomSheetHeaderPlainText growing a "title/body" variant,...

Should we create a new widget for the header in this PR, with a name like BottomSheetHeader? One distinction of this from BottomSheetHeaderPlainText is that the latter has a different styling than the title/body of this header widget.

(Thanks for the link to that interesting blog post! 🙂)

We shouldn't give translators control of the URL. If the Help Center page moves, we should only have to make a one-line change in our source code, instead of going through and changing language data for all the languages. 🙂

See web's string, can we do something like <z-link> there?

{num_of_people, plural, one {This message has been <z-link>read</z-link> by {num_of_people} person:} other {This message has been <z-link>read</z-link> by {num_of_people} people:}}

Also I think we should link to https://zulip.com/help/read-receipts instead of that page on CZO.

Also IIUC that plural syntax works, but I think we'd normally do

{count, plural, =1{1 person} other{{count} people}}

in case that's a more helpful model when translating into a language that varies the word order among the one/two/few/many/other cases? (I'm not actually sure if such languages exist actually.)

Yeah, in fact best to have the whole sentence inside the plural block — like it is in that example from web.

That way if e.g. the form of the verb needs to change too, the translator has that flexibility.

That way if e.g. the form of the verb needs to change too, the translator has that flexibility.

nit: IIUC the translator has that flexibility already; they can choose to write the translated string however they want, including by re-scoping the plural block (or even omitting it which would probably be wrong). But by putting the whole sentence in the plural block in English, we're potentially helping by minimizing the things a translator has to think through in order to make a correct translation for their language.

True. So it's more that we're giving them a model that makes it easier to see how to apply that flexibility.

We should store and check the org's "Enable read receipts" setting before including the button; see realm_enable_read_receipts in https://zulip.com/api/register-queue

How about

      overlayColor: WidgetStateColor.fromMap({
        WidgetState.pressed: designVariables.contextMenuItemBg.withFadedAlpha(0.20)
      }),

?

I updated my #1700 with that in response to #1700 (comment) 🙂 (there might be other things that would be helpful to bring over from ViewReactionsUserItem in my current revision there)

In the view-reactions sheet, I used 400 with a TODO(design) tune. I'd like to be consistent between the two places unless there's a reason not to, but I don't mind which value is used; 500 is fine if you think it's better.

Yeah. ThisSizedBox is a little bit different from the SizedBox in view-reactions sheet. Here, the height of the view-receipts sheet is 500, and in view-reactions sheet, the height of the user-item list is 400 (excluding the reactions-header and dismiss-button). In fact, the height of that sheet is 518px (I can make this sheet's size 518 to make it consistent). I wrapped SizedBox on the entire sheet (instead of the user-item list) so that when there are no read receipts, the sheet's height is not changed.

Maybe the same approach could be applied to view-reactions sheet to address the following comment:

This means we can end up fetching multiple times, so I think my ideal version would have generation logic like in lib/model/message_list.dart, to invalidate stale fetches. If results arrive out of order, we don't want to get stuck showing old results when we know we have results from a fetch we made more recently.

I think that can be a followup though—I can write up a more detailed proposal later, or perhaps just send a draft to demonstrate, once this work is merged.

What are the reasons we would want to do a new fetch because a dependency changed?

I think it may only be if there's a new store. So this could go in an onNewStore method instead.

Then the generation-like logic can just be: when getting the response, check if the store we had at the start of the request is still the current store (with identical). If not, ignore the response.

Sure, that's reasonable. In zulip-mobile I implemented polling to refresh this page every 15s (IIRC), and generic generation logic would help if we did that as a followup.

But arguably the polling wasn't really needed in zulip-mobile, and it's maybe less compelling in zulip-flutter, where the view is more clearly ephemeral, not something you'd necessarily expect to stay up-to-date if you left it foregrounded in the app. (A modal bottom sheet vs. a whole full-screen page with a back button.)

Let's link to the Figma, so it's easier for readers to check the implementation against the spec.

I think this is a rare case where I disagree with the Figma (link). Can we change the horizontal padding to 16 and the bottom padding to 4, like in BottomSheetHeaderPlainText (which in main follows this in the Figma)?

Here's a before/after of my proposal (from the current revision):

Before	After

You can leave a code comment explaining that we prefer consistency with that other header, with a link to this GitHub comment.

nit: we generally write zulipLocalizations instead of localizations; that way it's distinct from other localizations like MaterialLocalizations.

The Figma specifies some other details about the underline style. Let's follow any of those that we can (e.g. decorationThickness, I think) and write TODO(upstream)s for the others.

I think there's an important difference between the two thicknesses in Figma and Flutter, and I am not sure how we can map between them. In Figma, it's a percentage of the font size of the text to which the decoration belongs. In Flutter, it is a multiplier of the thickness defined by the font.

Odd, thanks for noticing that.

From Flutter's decorationThickness doc, it does seem like you can provide a constant (like 1.5 or 2.0 or whatever) and the thickness you actually get will change with the font size. That seems desirable.

But if it's true that font files always include a preferred underline stroke-width value (as that doc says), I haven't been able to find that value for the font we're using here, Source Sans 3. 🤷‍♂️

Hmm, I didn't find this in the list of Figma variables that I usually look at.

Turns out it's defined in the "Zulip Web UI kit": https://www.figma.com/design/msWyAJ8cnMHgOMPxi7BUvA/Zulip-Web-UI-kit?node-id=0-1&p=f&vars=1&var-id=b9c211a7c212568bc84139413454dd8dd07321fb%2F833-56&m=dev

It has distinct light/dark values; we should use both. And maybe add a comment like

    link: const Color(0xff066bd0), // From "Zulip Web UI kit"

How about:

?

It takes some nonlocal reasoning to work out which nav-transition this is about, and I notice that we're always passing the same duration here.

How about replacing prepareReceiptsResponse with prepareReceiptsResponseSuccess(List<int> userIds) and prepareReceiptsResponseError(Object? httpException), and control the delay in setupReceiptsSheet?

Can be two lines instead of four I think 🙂

      check(find.byWidgetPredicate((widget) => widget is ProfilePage && widget.userId == 1))
        .findsOne();

Relatedly: in general we never want to keep a reference to the store across an await and use it again after, because the old store might already have been disposed. Instead, after an await we should look up the store afresh using a context.

This parent/child structure feels odd; can we make the three parts be siblings instead?

      span = TextSpan(children: [
        TextSpan(text: match.group(1)),
        TextSpan(text: match.group(2), /* etc. */),
        TextSpan(text: match.group(3)),
      ]);

Thinking more about this, I think my preference is:

• Declare an API that's explicitly about "empty content". One sign of a nice API is that, when you read about it, it's clear early on whether it's something you want to use or not. This BottomSheetInfoText widget catches the attention of a would-be caller who wants to show some "info text", but then it's not clear whether this or BottomSheetHeaderPlainText is the better way to do that—especially because this says the style is modeled on that header widget :)
• Instead of BottomSheetInfoText, I suggest basically duplicating PageBodyEmptyContentPlaceholder, adapted for use in a bottom sheet, and with bidirectional links. Then:
  • "Empty content placeholder" answers the "should-I-use-this" question more quickly than "info text".
  • It actually sort of comes from a Figma frame, though indirectly 🙂

For example:

/// A "no content here" message for when a bottom sheet has no content to show.
///
/// Show this below a [BottomSheetHeader] if present.
///
/// See also:
///  * [PageBodyEmptyContentPlaceholder], for a similar element to use in
///    pages on the home screen.
// TODO(design) we don't yet have a design for this;
//   it was ad-hoc and modeled on [PageBodyEmptyContentPlaceholder].
class BottomSheetEmptyContentPlaceholder extends StatelessWidget {
  const BottomSheetEmptyContentPlaceholder({super.key, required this.message});

  final String message;

  @override
  Widget build(BuildContext context) {
    final designVariables = DesignVariables.of(context);

    return Padding(
      padding: EdgeInsets.fromLTRB(24, 48, 24, 16),
      child: Align(
        alignment: Alignment.topCenter,
        child: Text(
          textAlign: TextAlign.center,
          style: TextStyle(
            color: designVariables.labelSearchPrompt,
            fontSize: 17,
            height: 23 / 17,
          ).merge(weightVariableTextStyle(context, wght: 500)),
          message)));
  }
}

Here's how it would look:

Before	After

I'll push a draft commit with that example.